[Dataflow Streaming] [Multi Key] Introduce KeyGroupWorkQueue and integrate with BoundedQueueExecutor#38767
[Dataflow Streaming] [Multi Key] Introduce KeyGroupWorkQueue and integrate with BoundedQueueExecutor#38767arunpandianp wants to merge 5 commits into
Conversation
…oundedQueueExecutor
- Add Uint128Proto and key_group to WorkItem in windmill.proto.
- Update Work model to support key groups.
- Implement KeyGroupWorkQueue for grouping tasks by key group. KepGroupWorkQueue
is a FIFO queue that allows polling elements based on
global order and also by order within a key group.
- Integrate BoundedQueueExecutor with KepGroupWorkQueue and support targeted polling.
|
R: @scwhittle |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new KeyGroupWorkQueue implementation to support grouping and targeted polling of work items by key group. It updates the Work model and windmill.proto to include key group information and integrates this new queue mechanism into the BoundedQueueExecutor. These changes are gated behind an experimental flag, ensuring no impact on existing core logic. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for multi-key bundles in the Streaming Dataflow Worker, guarded by the unstable_enable_multi_key_bundle experiment. It adds a custom, thread-safe KeyGroupWorkQueue to allow polling work by computation and key group in FIFO order, along with corresponding updates to BoundedQueueExecutor, Work, and windmill.proto. Feedback on the new queue implementation suggests adding a defensive check in unlinkNode to prevent double-unlinking issues, and reassigning the result of checkStateNotNull in take() to satisfy static analysis nullness checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Run Java PreCommit |
| */ | ||
| public static final class KeyGroup { | ||
|
|
||
| // The default 0 key group. Work items with 0 keyGroup will always be executed |
There was a problem hiding this comment.
remove the 0s from the comment , don't think it clarifies anything
| @GuardedBy("this") | ||
| private long totalTimeMaxActiveThreadsUsed; | ||
|
|
||
| private final @Nullable KeyGroupWorkQueue keyGroupWorkQueue; |
There was a problem hiding this comment.
add comment,
// If set the keyGroupWorkQueue is used by the underlying executor.
| this.task = task; | ||
| if (task instanceof QueuedWork) { | ||
| this.computationId = ((QueuedWork) task).getWork().getComputationId(); | ||
| this.keyGroup = ((QueuedWork) task).getWork().getKeyGroup().orElse(null); |
There was a problem hiding this comment.
should we leave computationId unset if there is no keygroup? less possible states to worry about.
There was a problem hiding this comment.
The comment is on a stale change, now both exist or both does not.
| @Nullable Node next = curr.nextNode; | ||
| unlinkNode(curr); | ||
| Runnable task = curr.task; | ||
| if (task != null) { |
There was a problem hiding this comment.
see above, if task is non-null then we can simplify throughout
There was a problem hiding this comment.
done. Updated task to be a nonnull.
| class KeyGroupWorkQueue extends AbstractQueue<Runnable> implements BlockingQueue<Runnable> { | ||
|
|
||
| static class Node { | ||
| final @Nullable Runnable task; |
There was a problem hiding this comment.
instead of nullable how about singleton exception throwing runnable for the sentinels? Seems like it will simplify logic below
There was a problem hiding this comment.
Good idea! done.
| try { | ||
| int added = 0; | ||
| @Nullable Node curr = globalHead.nextNode; | ||
| while (curr != null && curr != globalTail && added < maxElements) { |
There was a problem hiding this comment.
throw exception if curr is null? ditto for others
scwhittle
left a comment
There was a problem hiding this comment.
mostly test stuff remaining
|
|
||
| /** An executor for executing work on windmill items. */ | ||
| @SuppressWarnings({ | ||
| "nullness" // TODO(https://github.com/apache/beam/issues/20497) |
There was a problem hiding this comment.
remove? might be able to remove some null checks below then too
| if (keyGroupWorkQueue == null) { | ||
| return null; | ||
| } | ||
| QueuedWork queuedWork = keyGroupWorkQueue.pollWork(computationId, keyGroup); |
| return new QueuedWork(work, executor.createBudgetHandle(1, workBytes)); | ||
| } | ||
|
|
||
| private static class MockRunnable implements Runnable { |
There was a problem hiding this comment.
nit: NoOpRunnable or FakeRunnable
| } | ||
|
|
||
| @Test | ||
| public void testMemoryPruningLeavesZeroLeaks() { |
There was a problem hiding this comment.
not sure what memorypruning means here or how leaks is being validated. Can you just add size checking to other tests and remove this one?
| } | ||
| if (task != null) { | ||
| consumedCount.incrementAndGet(); | ||
| } |
There was a problem hiding this comment.
if task is null, should we perform blocking take the next time?
| assertTrue(started.await(2, TimeUnit.SECONDS)); | ||
| // Give thread a moment to enter await() | ||
| Thread.sleep(100); | ||
| assertEquals(Thread.State.WAITING, t.getState()); |
There was a problem hiding this comment.
instead of sleep and then check, how about a loop while it is not in WAITING
ditto below
| t.setDaemon(true); | ||
| t.start(); | ||
|
|
||
| assertTrue(started.await(2, TimeUnit.SECONDS)); |
There was a problem hiding this comment.
higher timeouts here so it isn't flaky? ditto below
| () -> { | ||
| started2.countDown(); | ||
| try { | ||
| result2.set(queue.poll(2, TimeUnit.SECONDS)); |
There was a problem hiding this comment.
ditto higher so not flaky
| assertEquals(workA2, polledA2); | ||
| assertEquals(1, queue.size()); | ||
|
|
||
| // Poll with keyGroup2 again - should return null |
There was a problem hiding this comment.
trying polling a keygroup never added
The changes are behind an experiment
unstable_enable_multi_key_bundleand does not affect the default core logic.